feat(api): pages and widgets on api/2 - #3133

As far as I see, new routes are now added in the controllers, not in the routes.php.

Thanks @rjzondervan, agreed. The routes are now declared on the controller with #[ApiRoute], and this PR no longer touches appinfo/routes.php.

Do we agree that a data widget can only show a table or view that is already in the application? This seems to ignore that, and lets a page save add new tables to the application, which also gives every sharee of the application read access to them. The application owner also gets full rights to any table in the application 🤔

Yes, we agree. Saving a page no longer adds anything to the application. A data widget's source has to be {nodeRelId} of a table or view that's already in it; anything else is a 400, with the same message whether the table doesn't exist or just isn't in the application. The {nodeType, nodeId} form is gone, with its tests.

Could we check the permission with canManageContext($context, $userId), using the application we already loaded here?

so we don't have to find it first. Seems like that should work

Done, it uses canManageContext() with the application loaded just before.

We have the checks on the Controller layer, which should be sufficient. They can be dropped here to save some CPU cycles and DB queries, assuming that behind the external guards, internal handling is controlled and safe.

Doesn't #[RequirePermission] already does the same check.

It does. I removed the @CanManageContext annotation here and on destroy().

Doesn't #[RequirePermission] already does the same check?

Yes, removed here too.

Too long, which adds clutter. The code should be readable enough to not need long comments. Also, it's very easy for comments that reference code properties to become outdated and just confusing

Agreed. It's down to what the code can't tell an API user: what a widgets list does to the page, and what a widget looks like. I shortened the comments in the other files I touched as well.

No 403 why?

That's on purpose. Reading a page only needs read access, which everyone in the application has. Someone outside it gets a 404 from the permission middleware, like on the other application endpoints, so outsiders can't tell whether an application exists. I added that to the 404 description, so the API docs say it too.

Similar comment as at https://github.com/nextcloud/tables/pull/3131/changes#r4431715470 about all these tests

Done here as well: 44 test methods down to 25, and 677 test lines down to 544. The controller tests are now one data provider for the endpoints and one for the errors; the checks on route and rate limit attributes are gone.

string (or array) not JSON

This is not translatable, but sounds (end) user facing.

We have the checks on the Controller layer, which should be sufficient. They can be dropped here to save some CPU cycles and DB queries, assuming that behind the external guards, internal handling is controlled and safe.

could also result in a 409?

409 also here.

Is the index (here and elsewhere) to the end user? Also, all user facing messages have to be translateable.

A widgets list replaces all widgets of the page, and the only conflict this detects is a change that lands while the request runs. A save sent from a page loaded before another save went through is taken as it is: widgets the other save added are deleted and its changes to the listed widgets are overwritten, with a 200. In #3215 GridPage.vue copies the page into a draft when editing starts and sends every widget of the draft on save, so two people editing the same page (or one person in two tabs) undo each other's work without a message, and the window is the whole edit session.

Reproduced on the dev instance: a save that added a header returned 200; a second save built from the page before it, changing another widget's text, also returned 200, and the new header was gone. The one stale case that is caught, a listed widget the other save removed, is answered with "Widget 0 has the id of no other widget on this page." from the check below instead of the "Load it again and retry" message CHANGED_IN_BETWEEN has for it.

For saves that do overlap, saveChanges() decides by what the request asked for rather than by what failed: a request that renames the page and collides with another save on a widget's source key is told its new technical name is taken (only an API client sends both in one request), and any other database error passes through as a 500. Two saves that update the same widgets in a different order can deadlock, and so can two in the same order when one of them drops a widget the other keeps. Simulated on the dev instance with each save holding its first widget's row lock for a moment: one of the two failed in every run, and not even with the deadlock. MariaDB rolls back the victim's whole transaction, so rolling back to the savepoint of syncPage()'s nested atomic() fails with "SAVEPOINT DOCTRINE_2 does not exist", which replaces the original error and ends as a 500.

A version column on the page (the migration from #3131 isn't released yet), returned in TablesPage and sent back with the save, closes this: the transaction starts with UPDATE ... SET version = version + 1 WHERE id = ? AND version = ?, and 0 affected rows is the "load it again" answer (a 409, as blizzz suggests for saveChanges()). That one statement covers added, changed and removed widgets, serialises two saves of the same page through its row lock, and leaves saveChanges() only the renamed technical name to map.

The data type has no settings, and json_encode() writes its empty settings array as [], while TablesWidgetType and the generated spec declare an object (additionalProperties: WidgetSettingSchema). On the dev instance GET /api/2/widget-types returns "settings":[] for data and objects for the other two types, so a client generated from openapi.json that maps this to a dictionary (Kotlin, Swift, Dart) can't read the list, and a response validator flags it. The Vue code in the stack only gets by because Object.entries([]) is empty.

Widget::getSettingsObject() and the |\stdClass on TablesWidget.settings already handle this for a widget's own settings. Returning new \stdClass() here only when a type has no settings (typed array<string, TablesWidgetSettingSchema>|\stdClass), and asserting {} for data in testWidgetTypesListsEveryType(), fixes it; casting every schema to an object would lose the non-empty ones in OCS XML (see the comment on Widget::getSettingsObject()).

The generator turns list<array<string, mixed>> into additionalProperties: {type: object}: openapi.json says every field of a widget is an object, and openapi.ts types each one as Record<string, never>, so a client built from the spec can't send type: 'header' or gridX: 0 without a type error, and the widget shape (which fields, which are required, source: {nodeRelId}) exists only in the prose above.

The generator handles array shapes in parameters ($nodes of ContextController::importScheme() comes out with its fields and required list), so a shape such as list<array{id?: ?int, type: string, title?: ?string, icon?: ?string, description?: ?string, gridX: int, gridY: int, gridWidth: int, gridHeight: int, settings?: ?array<string, TablesWidgetValue>, source?: ?array{nodeRelId: int}}>, best as a named type in ResponseDefinitions (the internal TablesWidgetInput in PageService describes the parsed form), documents what the endpoint accepts. It would also be the natural input type for the value object discussed in #3132 (comment).

AGENTS.md asks for #[UserRateLimit(limit: 20, period: 60)] on mutation endpoints. create() and update() carry one, destroy() doesn't: on the dev instance 25 DELETEs in 16 seconds all returned 200, while the 21st POST within a minute got a 429.

Same gap as #3132 (comment), on the path users take most: on the dev instance POST .../pages with {"title": "\u00a0"} or {"title": "\u200b"} returns 200 and stores that title. In #3215 a page tab gets its name only from the title (pageTitle() returns page.title when it is truthy, the icon is aria-hidden), so such a page shows an empty tab that screen readers announce without a name, which the 1 to 200 characters rule here is meant to prevent.

OCA\Tables\Service\ValueObject\Title already refuses the \u00a0 case for tables, views and columns (ColumnService::normalizeTitle() wraps it; \u200b needs the addition mentioned there), so the shared helper suggested there would cover validateTitle() and validateDescription() here as well. Title counts bytes (strlen() > 200) while this counts characters, so reusing it as it is would shorten the limit for non-Latin titles.

#3211, stacked on this PR, removes this check again ("The page endpoint accepts widgets on the start page, which still cannot be deleted.") and turns testTheStartPageHoldsNoWidgets() into testTheStartPageHoldsWidgets(). So this 400 only exists between the two PRs, and if this one ships first, API clients see the rule flip between releases. Leaving it out here keeps the contract stable. The delete rule on line 172 then reads better as === Page::TYPE_STARTPAGE, which is what it means; it would also let a page of another type, which imports before #3131 could create, be deleted instead of being refused as the start page.

It also makes the suggestion on #3132 to skip start-page widgets in importWidget() (#3132 (comment)) moot: with #3211 the importer already does what the stack ends up with.

saveNamed() is saveChanges() with a technical name, minus the \InvalidArgumentException branch that an insert can't reach, so $this->saveChanges(fn () => $this->pageMapper->insert($page), $technicalName) in create() behaves the same and this method can go. The copy has already drifted: it drops the unique violation as previous exception, which saveChanges() keeps for the log. The "already used" message also exists three times (lines 303, 323, 339), and the translation blizzz asked for would have to cover each copy.

A start page gets a title here but can't lose it again: it starts as NULL (ContextService::insertPage()), {"title": "Start"} is accepted, and {"title": ""} is refused by validateTitle(), while an empty icon or description removes the value. The page bar in #3215 shows the translated "Home" only while the title is NULL, so once any client sets one, every user sees that string, in one language, for good. Reproduced on the dev instance (200, then 400). Letting '' reset the start page's title to NULL, or refusing a title for it, keeps the change reversible.

The removed getSettingsArray() turned a stored value that isn't a JSON object into {}, and the deleted WidgetTest case "not an object" covered that. With Types::JSON and a ?array property, Nextcloud's Entity setter assigns whatever json_decode() returns, so a stored scalar such as "text" now throws a TypeError while the row is hydrated (broken JSON still decodes to null and is fine). The app doesn't write such values itself, but a manual fix, a restored dump or a later migration can, and then the page can't be read, saved or deleted through this API (the controller doesn't catch \Error), and the exports that read its widgets fail too.

Reproduced on the dev instance by setting one widget's settings to "text": GET and DELETE .../pages/{id} answered 500 with "Cannot assign string to property OCA\Tables\Db\Widget::$settings of type ?array", and so did the application's scheme export.

Whatever replaces the JSON type after blizzz's comment above, an is_array() guard where the stored value is decoded keeps one bad row from locking the whole page.

The page endpoints return every widget's settings as a \stdClass, and Nextcloud's OCS XML renderer turns any \stdClass into an empty element (BaseResponse::toXML(): if ($v instanceof \stdClass) { $v = []; }). XML is what OCS answers when a client sends neither format=json nor Accept: application/json, so in that format GET, PATCH and DELETE .../pages/{pageId} return <settings/> for every widget and its heading, text and colours are gone. On the dev instance the same page gives {"text": "Edited by A"} for a text widget in JSON and <settings/> in XML.

Returning the sorted array when it has entries and new \stdClass() only when it is empty keeps the JSON output as it is (TablesWidget.settings already allows both) and keeps the values in XML.